Clean up logging for next major #1070
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Background
For a very long time, stripe-go logged by way of the `Printfer`
interface:
``` go
type Printfer interface {
Printf(format string, v ...interface{})
}
```
`Printfer` had a few problems: it didn't allow any kind of granularity
around logging levels, didn't allow us to behave well by sending errors
to `stderr` and other output to `stdout`, and wasn't interoperable with
any popular Go loggers (it was interoperable with the one in the
stdlib's `log`, but serious programs tend to move away from that).
A while back, I introduced the new `LeveledLogger` interface, which
corrected the deficiencies with `Printfer`.
We made quite a bit of effort to stay compatible with the existing
configuration options around logging like the `Logger` and `LogLevel`
globals, while also adding new ones for the leveled logger like
`DefaultLeveledLogger`.
The downside of that is it made reasoning about the different
combinations complicated. For example, you had to know that if both
`Logger` and `DefaultLeveledLogger` were set, then the former would take
precedence, which isn't necessarily obvious.
## Changes in this patch
Since we're on the verge of a major, I've taken the liberty of cleaning
up all the old logging infrastructure. This involves removing its types,
interfaces, helpers, global configuration options, and local
configuration options. We're left with a much simpler setup with just
`stripe.DefaultLeveledLogger` and `BackendConfig.LeveledLogger`.
Users who were already using the leveled logger (as recommended in the
README for sometime) won't have to change anything, nor will users who
had no logging configured. Users on the old infrastructure will have to
make some config tweaks, but ones that are quite easy.
And since we're changing logging things anyway: I've made a small tweak
such that if left unset by the user, the default logger now logs errors
only (instead of errors + informational). This is a more reasonable
default on Unix systems where programs are generally not expected to be
noisy over stdout. If a user wants to get informational messages back,
it's a very easy configuration tweak to `DefaultLeveledLogger`.
brandur-stripe
changed the base branch from
brandur-default-2-retries
to
integration-v71
richardm-stripe
approved these changes
Apr 16, 2020
|
Thank you Richard! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
For a very long time, stripe-go logged by way of the
Printferinterface:
Printferhad a few problems: it didn't allow any kind of granularityaround logging levels, didn't allow us to behave well by sending errors
to
stderrand other output tostdout, and wasn't interoperable withany popular Go loggers (it was interoperable with the one in the
stdlib's
log, but serious programs tend to move away from that).A while back, I introduced the new
LeveledLoggerinterface, whichcorrected the deficiencies with
Printfer.We made quite a bit of effort to stay compatible with the existing
configuration options around logging like the
LoggerandLogLevelglobals, while also adding new ones for the leveled logger like
DefaultLeveledLogger.The downside of that is it made reasoning about the different
combinations complicated. For example, you had to know that if both
LoggerandDefaultLeveledLoggerwere set, then the former would takeprecedence, which isn't necessarily obvious.
Changes in this patch
Since we're on the verge of a major, I've taken the liberty of cleaning
up all the old logging infrastructure. This involves removing its types,
interfaces, helpers, global configuration options, and local
configuration options. We're left with a much simpler setup with just
stripe.DefaultLeveledLoggerandBackendConfig.LeveledLogger.Users who were already using the leveled logger (as recommended in the
README for sometime) won't have to change anything, nor will users who
had no logging configured. Users on the old infrastructure will have to
make some config tweaks, but ones that are quite easy.
And since we're changing logging things anyway: I've made a small tweak
such that if left unset by the user, the default logger now logs errors
only (instead of errors + informational). This is a more reasonable
default on Unix systems where programs are generally not expected to be
noisy over stdout. If a user wants to get informational messages back,
it's a very easy configuration tweak to
DefaultLeveledLogger. I thinkthis change is okay for the reason above, but also because I don't think
a lot of thought was ever put into the original default -- we've just
been cargo culting through releases for a long time now.
r? @ob-stripe @remi-stripe
cc @stripe/api-libraries
Note: Targets the branch in #1055 for V71 integration.